Conversation
|
Can I get a review/approval for this PR please? It's just a port of #5791 (which I reviewed and was almost ready to go in) with the out artifacts cleaned up |
|
|
||
| public void CumulativeMax(PrimitiveColumnContainer<DateTime> column) | ||
| { | ||
| var ret = column.Buffers[0].ReadOnlySpan[0]; |
There was a problem hiding this comment.
I had the same thought when I first saw the PR, so I looked at what the other columns are doing. None of them check for empty here. It's not high priority IMO, so I'm thinking we can fix that for all the columns in a separate PR?
There was a problem hiding this comment.
Can you log an issue for this? So we remember to do it.
| if (value != null) | ||
| { | ||
| value = Convert.ChangeType(value, column.DataType); | ||
| try |
There was a problem hiding this comment.
How expensive is this try-catch? It is inside a loop, so it may effect perf.
Codecov Report
@@ Coverage Diff @@
## main #5834 +/- ##
========================================
Coverage 68.35% 68.36%
========================================
Files 1131 1134 +3
Lines 241210 241856 +646
Branches 25039 25110 +71
========================================
+ Hits 164887 165333 +446
- Misses 69819 70006 +187
- Partials 6504 6517 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| { | ||
| value = Convert.ChangeType(value, column.DataType); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
This should be catching FormatException. Or rather, maybe Append's caller should catch this exception
There was a problem hiding this comment.
Alright, I'm going to rip this try-catch out of this PR. We can add it later if we want to. I'm more interested in getting the DateTime support in
|
|
||
| public void CumulativeMax(PrimitiveColumnContainer<DateTime> column) | ||
| { | ||
| var ret = column.Buffers[0].ReadOnlySpan[0]; |
There was a problem hiding this comment.
Can you log an issue for this? So we remember to do it.
ghost
locked as resolved and limited conversation to collaborators
3 participants
Clean up the remaining files from #5791
cc @derekdiamond